Skip to content

flashbench: 2012-06-06 -> 2020-01-23#112897

Closed
Luflosi wants to merge 5 commits intoNixOS:masterfrom
Luflosi:update/flashbench
Closed

flashbench: 2012-06-06 -> 2020-01-23#112897
Luflosi wants to merge 5 commits intoNixOS:masterfrom
Luflosi:update/flashbench

Conversation

@Luflosi
Copy link
Copy Markdown
Contributor

@Luflosi Luflosi commented Feb 12, 2021

Motivation for this change

The current version of flashbench is quite old. I updated it to use the latest commit from about a year ago. There have only been four new commits with small changes since 2012, so I don't expect any regressions.
I also switched from fetchgit to fetchGitHub.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested a review from rycee February 12, 2021 12:47
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Feb 12, 2021
@r-rmcgibbo
Copy link
Copy Markdown

r-rmcgibbo commented Feb 12, 2021

Result of nixpkgs-review pr 112897 at ee7a1d1b run on x86_64-linux 1

1 package built:
2 suggestions:
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall and runHook postInstall.

    Near pkgs/os-specific/linux/flashbench/default.nix:14:3:

       |
14 |   installPhase = ''
   |   ^
  • warning: unclear-gpl

    gpl2 is a deprecated license, check if project uses gpl2Plus or gpl2Only and change meta.license accordingly.

    Near pkgs/os-specific/linux/flashbench/default.nix:25:5:

       |
25 |     license = licenses.gpl2;
   |     ^

@r-rmcgibbo
Copy link
Copy Markdown

r-rmcgibbo commented Feb 12, 2021

Result of nixpkgs-review pr 112897 at ee7a1d1b run on aarch64-linux 1

1 package built:
2 suggestions:
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall and runHook postInstall.

    Near pkgs/os-specific/linux/flashbench/default.nix:14:3:

       |
14 |   installPhase = ''
   |   ^
  • warning: unclear-gpl

    gpl2 is a deprecated license, check if project uses gpl2Plus or gpl2Only and change meta.license accordingly.

    Near pkgs/os-specific/linux/flashbench/default.nix:25:5:

       |
25 |     license = licenses.gpl2;
   |     ^

@Luflosi
Copy link
Copy Markdown
Contributor Author

Luflosi commented Feb 12, 2021

I changed the license from gpl2 to gpl2Only.

@Luflosi
Copy link
Copy Markdown
Contributor Author

Luflosi commented Feb 12, 2021

Should I add the runHooks to the installPhase? I'm not convinced, it's actually useful here.

@SuperSandro2000
Copy link
Copy Markdown
Member

SuperSandro2000 commented Feb 12, 2021

Should I add the runHooks to the installPhase? I'm not convinced, it's actually useful here.

Otherwise the phases are not run in overlays which is usually expected.

@Luflosi
Copy link
Copy Markdown
Contributor Author

Luflosi commented Feb 12, 2021

Thanks for the explanation, I added them. Please review.

@rmcgibbo rmcgibbo mentioned this pull request Feb 12, 2021
Merged
SuperSandro2000
SuperSandro2000 reviewed Feb 12, 2021
View reviewed changes
@rycee
Copy link
Copy Markdown
Member

rycee commented Feb 12, 2021

Thanks! Squashed and rebased to master in ba68041. Note, I moved the "unstable" to pname since it is part of the name, not the version.

@rycee rycee closed this Feb 12, 2021
@Luflosi Luflosi deleted the update/flashbench branch February 12, 2021 19:38
@SuperSandro2000
Copy link
Copy Markdown
Member

SuperSandro2000 commented Feb 12, 2021

Thanks! Squashed and rebased to master in ba68041. Note, I moved the "unstable" to pname since it is part of the name, not the version.

Thats wrong. We only have one flashbench package so this part belongs to the version.

@rycee
Copy link
Copy Markdown
Member

rycee commented Feb 13, 2021

I disagree, it is always part of the name:

nix-repl> builtins.parseDrvName "flashbench-unstable-2020-01-23"
{ name = "flashbench-unstable"; version = "2020-01-23"; }

It may make intuitive sense to put the "unstable" in the version field but I think this is misleading since it doesn't match the package name and version that Nix sees.

@SuperSandro2000
Copy link
Copy Markdown
Member

SuperSandro2000 commented Feb 14, 2021 •
edited
Loading

It may make intuitive sense to put the "unstable" in the version field but I think this is misleading since it doesn't match the package name and version that Nix sees.

That function is by design broken and needs a fix. The last hundred or so PRs I reviewed where done with the unstable in the version number because people tend to use pname all over the place.

See #68518 and https://repology.org/projects/?search=flashbench which follow no version number at all.

@rycee
Copy link
Copy Markdown
Member

rycee commented Feb 14, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@rycee rycee Awaiting requested review from rycee

Assignees

No one assigned

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Luflosi @r-rmcgibbo @SuperSandro2000 @rycee